[DPE-6468] feat: add connect_client data interface#205
Conversation
imanenami
requested review from
deusebio,
dragomirp,
marcoppenheimer,
taurus-forever and
welpaolo
imanenami
force-pushed
the
feat/dpe-6468
branch
from
af4b561 to
f27e0a5
Compare
deusebio
left a comment
deusebio
left a comment
There was a problem hiding this comment.
The changes are fine. I'm just wondering if we should raise a PR to the charm-relation-interface before merging this?
taurus-forever
left a comment
taurus-forever
left a comment
There was a problem hiding this comment.
LGTM for the code. +1 to Dragomir: AFAIK, all new interfaces need description on https://github.com/canonical/charm-relation-interfaces/ (c) Mykola.
The final decision here in on @deusebio .
|
Thanks, PR for |
|
Yes, as I mentioned in my comment earlier, we should land the charm-relation-interface PR first. I have already provided some comments there. Please have a look |
deusebio
left a comment
deusebio
left a comment
There was a problem hiding this comment.
Anyhow, the code here is totally fine! Just wait for the other PR to land to make sure that there are no comments there to be addressed in the implementation here. Thanks!
imanenami
force-pushed
the
feat/dpe-6468
branch
2 times, most recently
from
40ed0ab to
7c82de2
Compare
sinclert-canonical
left a comment
sinclert-canonical
left a comment
There was a problem hiding this comment.
I was not aware of this effort from 5 months ago, but it seems to be in good shape.
I noticed that the KafkaConnect family of classes lack the support of custom roles that got recently introduced in all other products (MongoDB, MySQL, PostgreSQL, OpenSearch, Kafka & Karapace). Is this a deliberate decision?
- If so, could you share why? AFAIK, Mykola wanted custom roles support product wide.
- If not, please take a look at how Marc introduced the custom roles support to the Karapace classes, once they were already defined on a previous PR. You can use it as a reference: #238
Disclaimer:
Please be sure I will be extraordinary fast to approve this PR once my concern gets clarified (either way). I just want to get an answer in case the custom-roles effort got overlooked. The last thing I want to do is to block a 5 months ago effort, being added as a reviewer last minute 😅
sinclert-canonical
left a comment
sinclert-canonical
left a comment
There was a problem hiding this comment.
Clarified via MatterMost: ACLS and permissions are not inherently supported by KafkaConnect.
Co-authored-by: Sinclert Pérez <sinclert.perez@canonical.com>
5 participants
Changes:
connect_clientrequirer and provider data interfaceskafka-connect-charmtest charm with an implementation ofKafkaConnectProvidesKafkaConnectRequirestoapplication-charm